Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix hls & dash segments cleanup. #4161

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

suzp1984
Copy link
Contributor

@suzp1984 suzp1984 commented Aug 29, 2024

Impacted Version

Both v6 and v7.

How to reproduce?

  1. go to develop or 6.0release branch.
  2. compiling srs
  3. ./objs/srs -c conf/hls.conf
  4. ffmpeg -re -i ./doc/source.flv -c copy -f flv rtmp://localhost/live/livestream publish a stream for more than 10 seconds to generate the .m3u8 and .ts file.
  5. type q to quite ffmpeg push stream.
  6. check ./objs/nginx/html/live/ folder, whether the .m3u8 and .ts files still exist.

the expected behavior is that the .m3u8 and .ts files should cleanup a few seconds later after rtmp stop publish.

Cause

Introduced by #4097

The hls segments cleanup process is running in this way previously.

srs_error_t SrsLiveSourceManager::notify(int event, srs_utime_t interval, srs_utime_t tick)
{
srs_error_t err = srs_success;
std::map< std::string, SrsSharedPtr<SrsLiveSource> >::iterator it;
for (it = pool.begin(); it != pool.end();) {
SrsSharedPtr<SrsLiveSource>& source = it->second;
// Do cycle source to cleanup components, such as hls dispose.
if ((err = source->cycle()) != srs_success) {
SrsContextId cid = source->source_id();
if (cid.empty()) cid = source->pre_source_id();
return srs_error_wrap(err, "source cycle, id=[%s]", cid.c_str());
}
// When source expired, remove it.
// @see https://github.com/ossrs/srs/issues/713
if (source->stream_is_dead()) {
SrsContextId cid = source->source_id();
if (cid.empty()) cid = source->pre_source_id();
srs_trace("Live: cleanup die source, id=[%s], total=%d", cid.c_str(), (int)pool.size());
pool.erase(it++);
} else {
++it;
}
}
return err;
}

A timer run above method in period, source->cycle() will run the hls and dash dispose() method to cleanup segment files.
But the cycle() will exit before run dispose, after #4097 introduced the enabled variable.

    if (!enabled) {
        return err;
    }

Solution

run source->dispose(); before destroy SrsLiveSource.

@winlinvip winlinvip added the EnglishNative This issue is conveyed exclusively in English. label Aug 29, 2024
@winlinvip
Copy link
Member

winlinvip commented Oct 15, 2024

In the source cycle, it will call hls dispose:

srs_error_t SrsLiveSourceManager::notify(int event, srs_utime_t interval, srs_utime_t tick) {
        // Do cycle source to cleanup components, such as hls dispose.
        if ((err = source->cycle()) != srs_success) {

srs_error_t SrsOriginHub::cycle() {
    if ((err = hls->cycle()) != srs_success) {
        return srs_error_wrap(err, "hls cycle");
    }

srs_error_t SrsHls::cycle() {
    if (srs_get_system_time() - last_update_time <= hls_dispose) {
        return err;
    }

    dispose();

The stream dead detection will check the delay cleanup:

srs_error_t SrsLiveSourceManager::notify(int event, srs_utime_t interval, srs_utime_t tick)
        if (source->stream_is_dead()) {

bool SrsLiveSource::stream_is_dead() {
    if (now < stream_die_at_ + hub->cleanup_delay()) {
        return false;
    }

srs_utime_t SrsOriginHub::cleanup_delay() {
    srs_utime_t hls_delay = hls->cleanup_delay();

srs_utime_t SrsHls::cleanup_delay()
{
    if (!enabled) {
        return 0;
    }

    // We use larger timeout to cleanup the HLS, after disposed it if required.
    return _srs_config->get_hls_dispose(req->vhost) * 1.1;
}

After FFmepg quit, the HLS is not enabled, so it also disable the dispose. I think we should fix this issue by modify SrsHls::cleanup_delay rather than call hls dispose again.

In general, we should think about how to dispose a !enabled stream source in cycle, not dispose it again before destroy it.

@cdgraff
Copy link

cdgraff commented Nov 19, 2024

+1 I reach here for the same issue, some timeline to fix this? or merge this changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EnglishNative This issue is conveyed exclusively in English.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants